fix(preview): add preview sdk load check #4391

bfoxx1906 · 2025-12-10T21:00:06Z

This PR fixes an edge case where if the file info for the preview is loaded but the preview sdk is not, then the ContentPreview gets stuck in a perpetual loading state. This can happen on clients with increased network latency. When the function shouldLoadPreview executes on componentDidUpdate it checks to see whether or not the file is loaded and if it's new relative to the preview. If it is then loadPreview is set to true and which leads ContentPreview to attempt to load the preview. If the preview sdk is not loaded then loadPreview will return without actually calling the preview sdk and showing the preview. This leads to the perpetual loading state because shouldLoadPreview, when it's called again in componentDidUpdate, will always return false because the file will no longer be considered new relative to the component's state.

To fix this, I added a new field to the ContentPreview which keeps track of the preview sdk loaded state. This is the basis of a new conditional in shouldLoadPreview PR Ref that will return true and ultimately cause the preview to load if the preview sdk wasn't previously loaded but currently is.

Before

After

Before Screenshot: The logpoint pointed to by the red arrow shows where loadPreview gets called but the preview sdk isn't loaded.

After Screenshot: The arrow on the top points to the logpoint message that gets logged in loadPreview if the preview is trying to load but the sdk isn't ready. The arrow on the bottom points to the logpoint messages that get logged when shouldLoadPreview returns true because the preview sdk wasn't ready before but is now and then when the preview sdk actually gets called in the loadPreview function.

Summary by CodeRabbit

Bug Fixes
- Prevents premature preview load attempts until the external preview library is available and triggers a one-time load when it becomes available.
Tests
- Added tests verifying preview load gating behavior and that no load attempts occur when prerequisites (library, file, or token) are missing.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

coderabbitai · 2025-12-10T21:00:14Z

Walkthrough

Adds a guard to ContentPreview that tracks external preview library availability via a new previewLibraryLoaded boolean, preventing preview loads until the library is present and ensuring a single load when the library becomes available after a file is set.

Changes

Cohort / File(s)	Summary
Preview library guard implementation `src/elements/content-preview/ContentPreview.js`	Adds `previewLibraryLoaded: boolean` (initialized false). Updates `shouldLoadPreview` to trigger when the library becomes available and a preview hasn't been loaded. Recomputes and synchronizes `previewLibraryLoaded` in `loadPreview` and related lifecycle flows to avoid premature or duplicate loads.
Test coverage for library guard `src/elements/content-preview/__tests__/ContentPreview.test.js`	Adds tests verifying `shouldLoadPreview` returns true when the library transitions to available and no preview loaded. Adds tests ensuring `loadPreview` does not call `getFileId` when library is unavailable, file is unset, or token is unset.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Review synchronization of previewLibraryLoaded updates across lifecycle methods to avoid race conditions.
Verify shouldLoadPreview correctly reflects transitions from false→true and prevents repeated loads.
Confirm tests cover the introduced guard and edge conditions (missing file/token).

Poem

🐰 I waited, watched the library bloom,
Now preview waits no more — it springs from its room.
One gentle load when the signal turns green,
No frantic retries, no glimpses unseen.
Hooray for tidy hops in the previewing team!

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title directly and clearly describes the main change: adding a preview SDK load check to fix the perpetual loading state issue.
Description check	✅ Passed	The description is comprehensive, explaining the bug, the root cause, the solution, and includes visual evidence with before/after screenshots.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✨ Finishing touches

📝 Generate docstrings

🧪 Generate unit tests (beta)

Create PR with unit tests
Post copyable unit tests in a comment

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 22f5332 and 59cbe2a.

📒 Files selected for processing (1)

src/elements/content-preview/__tests__/ContentPreview.test.js (2 hunks)

🚧 Files skipped from review as they are similar to previous changes (1)

src/elements/content-preview/tests/ContentPreview.test.js

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)

GitHub Check: lint_test_build
GitHub Check: Summary

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

coderabbitai

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 454ede0 and 22f5332.

📒 Files selected for processing (2)

src/elements/content-preview/ContentPreview.js (3 hunks)
src/elements/content-preview/__tests__/ContentPreview.test.js (2 hunks)

🧰 Additional context used

🧬 Code graph analysis (2)

src/elements/content-preview/__tests__/ContentPreview.test.js (1)

src/elements/content-preview/ContentPreview.js (3)

props (291-301)

props (322-322)

props (377-377)

src/elements/content-preview/ContentPreview.js (6)

src/elements/content-preview/__tests__/ContentPreview.test.js (1)

file (18-18)

src/elements/content-sidebar/__tests__/ActivitySidebar.rtl.test.js (1)

file (74-79)

src/elements/content-sidebar/__tests__/ActivitySidebar.test.js (1)

file (49-54)

src/api/__tests__/Feed.test.js (1)

file (349-364)

src/elements/content-sidebar/__tests__/Sidebar.test.js (1)

file (28-33)

src/api/File.js (1)

file (229-229)

🪛 Biome (2.1.2)

src/elements/content-preview/ContentPreview.js

[error] 231-231: return types can only be used in TypeScript files

remove this type annotation

(parse)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)

GitHub Check: Summary

🔇 Additional comments (6)

src/elements/content-preview/ContentPreview.js (3)

231-232: LGTM!

The new previewLibraryLoaded property correctly tracks the preview library's load state. The static analysis hint about TypeScript syntax is a false positive—this is valid Flow syntax (the file has @flow directive on line 2).

457-462: LGTM! Good fix for the race condition.

The guard correctly detects when the preview library becomes available after the file was already set, ensuring the preview load is retried. The !this.preview check appropriately prevents re-triggering if a preview instance already exists.

The side effect (setting previewLibraryLoaded = true) in a should* method is slightly unconventional, but the comment clearly explains the intent and it's a pragmatic solution for synchronizing state.

820-822: LGTM!

Updating previewLibraryLoaded before the guard ensures the instance property stays synchronized with the actual library state. This allows shouldLoadPreview to correctly detect when the library becomes available on subsequent updates if loadPreview returns early here.

src/elements/content-preview/__tests__/ContentPreview.test.js (3)

178-185: LGTM!

Good test coverage for the new guard logic. It correctly verifies both the return value and the side effect of setting previewLibraryLoaded to true.

505-514: LGTM!

Good test for the early return behavior. Using getFileId as a proxy to verify execution stopped early is a reasonable approach since it's called after the guard check.

525-533: LGTM!

Good test coverage for the token validation in the guard. Correctly verifies that loadPreview returns early when the token is not set.

src/elements/content-preview/__tests__/ContentPreview.test.js

Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>

mergify · 2025-12-11T00:59:34Z

fix(preview): add preview sdk load check

1b368ad

Merge branch 'master' into previewsdk-not-loaded-fix

22f5332

bfoxx1906 marked this pull request as ready for review December 10, 2025 22:12

bfoxx1906 requested review from a team as code owners December 10, 2025 22:12

coderabbitai bot reviewed Dec 10, 2025

View reviewed changes

src/elements/content-preview/__tests__/ContentPreview.test.js Outdated Show resolved Hide resolved

jmcbgaston previously approved these changes Dec 10, 2025

View reviewed changes

Update src/elements/content-preview/__tests__/ContentPreview.test.js

59cbe2a

Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>

bfoxx1906 dismissed jmcbgaston’s stale review via 59cbe2a December 10, 2025 22:47

jmcbgaston approved these changes Dec 10, 2025

View reviewed changes

JChan106 approved these changes Dec 11, 2025

View reviewed changes

bfoxx1906 added the ready-to-merge label Dec 11, 2025

mergify bot added the queued label Dec 11, 2025

mergify bot merged commit 5555899 into box:master Dec 11, 2025
8 checks passed

mergify bot removed the queued label Dec 11, 2025

coderabbitai bot mentioned this pull request Jan 6, 2026

feat(content-preview): pass preloadUrlMap to enable prefetched image … #4411

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix(preview): add preview sdk load check #4391

fix(preview): add preview sdk load check #4391

Uh oh!

bfoxx1906 commented Dec 10, 2025 •

edited by coderabbitai bot

Loading

Uh oh!

coderabbitai bot commented Dec 10, 2025 •

edited

Loading

Uh oh!

coderabbitai bot left a comment

Uh oh!

Uh oh!

Uh oh!

mergify bot commented Dec 11, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

fix(preview): add preview sdk load check #4391

fix(preview): add preview sdk load check #4391

Uh oh!

Conversation

bfoxx1906 commented Dec 10, 2025 • edited by coderabbitai bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary by CodeRabbit

Uh oh!

coderabbitai bot commented Dec 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Estimated code review effort

Poem

Pre-merge checks and finishing touches

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

mergify bot commented Dec 11, 2025

Merge Queue Status

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

bfoxx1906 commented Dec 10, 2025 •

edited by coderabbitai bot

Loading

coderabbitai bot commented Dec 10, 2025 •

edited

Loading